Fix bypassing target admin delay using execute#6388
Merged
Amxx merged 5 commits intoOpenZeppelin:masterfrom Mar 3, 2026
Merged
Fix bypassing target admin delay using execute#6388Amxx merged 5 commits intoOpenZeppelin:masterfrom
Amxx merged 5 commits intoOpenZeppelin:masterfrom
Conversation
🦋 Changeset detectedLatest commit: dc7e2de The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
WalkthroughA new test case was added to the AccessManager test suite that validates the ability to execute calls that bypass a target's admin delay when updating authority. The test deploys an authority contract, applies an admin delay to the target, advances time beyond the setback period, verifies that a direct updateAuthority call is blocked with a NotScheduled error, then uses execute to invoke setAuthority on the target and confirms the operation succeeds by checking for an AuthorityUpdated event. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/access/manager/AccessManager.test.js (1)
1686-1686: UseMINSETBACKinstead of duplicating5n * 86400n.This keeps the test aligned with the canonical constant and avoids drift.
♻️ Proposed cleanup
- await time.increaseBy.timestamp(5n * 86400n, true); // minSetBack is 5 days + await time.increaseBy.timestamp(MINSETBACK, true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/access/manager/AccessManager.test.js` at line 1686, Replace the hardcoded duration 5n * 86400n in the test call to time.increaseBy.timestamp with the canonical constant MINSETBACK to avoid duplication; update the test to use MINSETBACK (e.g., time.increaseBy.timestamp(MINSETBACK, true)) and ensure MINSETBACK is referenced/imported the same way other tests use it so the test stays synced with the source constant (target the line containing time.increaseBy.timestamp and the test file's imports/consts).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/access/manager/AccessManager.test.js`:
- Around line 1681-1703: The test currently asserts that calling
this.manager.connect(this.admin).execute(this.target,
this.target.interface.encodeFunctionData('setAuthority', [newAuthority.target]))
succeeds, which preserves the bug; change the expectation to require a revert
with the same AccessManagerNotScheduled error (or appropriate custom error) and
assert that the target's authority did not change (e.g., call
this.target.authority() or the relevant getter and expect it to equal the
original authority), and remove the .to.emit(...
'AuthorityUpdated')/.withArgs(newAuthority) assertions; keep references to
$_setTargetAdminDelay, getTargetAdminDelay, updateAuthority, execute,
setAuthority, this.manager, this.target and newAuthority to locate the code.
---
Nitpick comments:
In `@test/access/manager/AccessManager.test.js`:
- Line 1686: Replace the hardcoded duration 5n * 86400n in the test call to
time.increaseBy.timestamp with the canonical constant MINSETBACK to avoid
duplication; update the test to use MINSETBACK (e.g.,
time.increaseBy.timestamp(MINSETBACK, true)) and ensure MINSETBACK is
referenced/imported the same way other tests use it so the test stays synced
with the source constant (target the line containing time.increaseBy.timestamp
and the test file's imports/consts).
ernestognw
approved these changes
Mar 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes H-01
Note: with these cahnges, we could technically get rid of the "updateAuthority" function and just use execute instead. That would be a breaking change. Maybe for 6.0 ???
PR Checklist
npx changeset add)